Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i18n: Relax sprintf arguments type #21919

Merged
merged 3 commits into from
Apr 27, 2020
Merged

i18n: Relax sprintf arguments type #21919

merged 3 commits into from
Apr 27, 2020

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Apr 27, 2020

Description

Sprintf accepts a number of different inputs. The named object of replacements seems particularly important:

sprintf( "hello %(target)s", { target: "world" } );

This is currently unsupported by the type ( format: string, ...args: string[] ) => string, which requires a 0 or more strings to be passed after the format.

Additionally, replacements may be a value for interpolation or a nullary function that returns a value for interpolation.

Remove the string restriction on replacement args. This aligns with what's currently on DefinitelyTyped.

Other considerations

With TypeScript function overloads, we can write the following:

import { sprintf as sprintfjs } from "sprintf-js";

type ReplacementValue = string | number; // We could accept more types, this is questionable.
type Replacement = ReplacementValue | (() => ReplacementValue);

function sprintf(format: string, ...replacements: Replacement[]): string;
function sprintf(
  format: string,
  replacementMap: Record<string, Replacement>
): string;
function sprintf(
  format: string,
  ...replacements: Replacement[] | [Record<string, Replacement>]
): string {
  return sprintfjs(format, ...replacements);
}
// Demo
console.log("=== These work fine ===");
console.log(sprintf("%s %s", "hello", "world"));
console.log(sprintf("%2$s %1$s", () => "world", "hello"));
console.log(sprintf("hello %(target)s", { target: "world" }));
console.log(sprintf("It's over %1$d!", () => 9000));
console.log(sprintf("It's over %(power)f!", { power: () => 9000 }));

console.log("=== Type errors ===");
console.log(sprintf("Hello %s", "world", { bad: "map" }));
console.log(sprintf("Hello %s", null));
console.log(sprintf("Hello %s", false));
console.log(sprintf("Hello %s", undefined));
console.log(sprintf("hello %(target)s", { target: "world" }, "extra-string"));
console.log(sprintf("It's over %1$d!", () => null));
console.log(sprintf("It's over %(power)s!", { power: () => undefined }));

// This is questionable, but not a type error.
console.log(sprintf("Hello %s")); // We can't actually prevent this empty `...args`

However, this seems to be poorly supported in TypeScript declarations via JSDoc: microsoft/TypeScript#25590

Here's my JSDoc translation, but it errors saying the function type must match the function signature:

/**
 * @typedef { string | number } ReplacementValue
 * @typedef { ReplacementValue | ( () => ReplacementValue) } Replacement
 * @typedef { ( format: string, ...replacements: Replacement[] ) => string } SprintFPositional
 * @typedef { ( format: string, replacementMap: Record<string,Replacement>, ...rest: never[] ) => string } SprintFNamed
 */

/**
 * Returns a formatted string. If an error occurs in applying the format, the
 * original format string is returned.
 *
 * @type { SprintFPositional & SprintFNamed }
 * @param {  string} format
 * @param {...any} args
 * @return {string}
 */
function sprintf() { /* snip */ }

Noticed in Automattic/wp-calypso#41207.

Types of changes

Bug fix: Relax rest args type of sprintf to any[].

Sprintf accepts a any number of positional arguments
_or_ a map object of named arguments.

Remove the string type.
@github-actions
Copy link

github-actions bot commented Apr 27, 2020

Size Change: +13 B (0%)

Total Size: 817 kB

Filename Size Change
build/block-editor/index.js 106 kB +13 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.08 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.23 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 761 B 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/editor-rtl.css 7.05 kB 0 B
build/block-library/editor.css 7.05 kB 0 B
build/block-library/index.js 112 kB 0 B
build/block-library/style-rtl.css 7.14 kB 0 B
build/block-library/style.css 7.14 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 179 kB 0 B
build/components/style-rtl.css 16.9 kB 0 B
build/components/style.css 16.9 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.42 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 3.54 kB 0 B
build/edit-navigation/style-rtl.css 485 B 0 B
build/edit-navigation/style.css 485 B 0 B
build/edit-post/index.js 27.8 kB 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/index.js 11 kB 0 B
build/edit-site/style-rtl.css 5.26 kB 0 B
build/edit-site/style.css 5.25 kB 0 B
build/edit-widgets/index.js 8.33 kB 0 B
build/edit-widgets/style-rtl.css 5 kB 0 B
build/edit-widgets/style.css 5 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 43.4 kB 0 B
build/editor/style-rtl.css 3.27 kB 0 B
build/editor/style.css 3.27 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.63 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.67 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@sirreal sirreal marked this pull request as ready for review April 27, 2020 14:03
@sirreal sirreal requested a review from swissspidy as a code owner April 27, 2020 14:03
@sirreal sirreal requested review from aduth and gziolo April 27, 2020 14:03
@sirreal
Copy link
Member Author

sirreal commented Apr 27, 2020

@alshakero @andrewserong This is relevant for Automattic/wp-calypso#41207

@sirreal sirreal changed the title i18n: Open sprintf function type i18n: Relax sprintf arguments type Apr 27, 2020
@sirreal sirreal self-assigned this Apr 27, 2020
@sirreal sirreal added [Package] i18n /packages/i18n [Type] Code Quality Issues or PRs that relate to code quality labels Apr 27, 2020
Copy link
Contributor

@alshakero alshakero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I had some doubts about the added value of {...*} but it's certainly better than nothing.

https://stackoverflow.com/a/4841722/1247990

@sirreal sirreal merged commit 6957a7b into master Apr 27, 2020
@sirreal sirreal deleted the update/types-i18n-sprintf branch April 27, 2020 17:02
@github-actions github-actions bot added this to the Gutenberg 8.0 milestone Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] i18n /packages/i18n [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants